Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] untangling auth-related code #5925

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

Really (REALLY) work in progress; slowly untangling some of the auth-code which was wrapper-upon-wrapper-upon-wrapper; often because types like registry.IndexInfo or registry.RepositoryInfo were part of the signature.

Docker Content Trust added yet-another layer of abstraction on top of that, with trust.ImageRefAndAuth, which is a wrapper on its own to wrap all those bits.

In most cases, all we need is;

  • either the name of the registry, or an image-ref from which we can deduct the name
  • we DONT need to know about Mirrors, because the client doesn't configure those
  • for most situations we don't even need to know about "insecure registries", but we can deduct "defaults" there from the hostname (default is loopbacks are insecure, everything else isn't)

And of course, there's the "special cases" for docker hub;

  • docker.io or index.docker.io PREFIX means "docker hub registry" (actual registry is registry-1.docker.io (but there's other domains possible ⚠️ we still need to normalise those)
  • we currently use https://index.docker.io/v1/ as KEY to store credentials for those
  • ☝️ also something we should consider changing, because for other registries, we use hostname without scheme / path

But there's more to untangle, such as creds-helpers/stores converting "to hostname", but other paths don't, and likely corner-cases, where (e.g.) a trailing / is missing in https://index.docker.io/v1/, etc etc.

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

FWIW, more untangling also happening in #5876 and #5921

I should probably look at basing this one on that, but wanted to see things before that

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 41.55844% with 45 lines in your changes missing coverage. Please review.

Project coverage is 59.05%. Comparing base (23d7346) to head (59e6e83).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5925      +/-   ##
==========================================
- Coverage   59.09%   59.05%   -0.04%     
==========================================
  Files         355      355              
  Lines       29751    29787      +36     
==========================================
+ Hits        17582    17592      +10     
- Misses      11193    11218      +25     
- Partials      976      977       +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thaJeztah thaJeztah force-pushed the simplify_auth branch 3 times, most recently from df692c7 to 8a4f9b4 Compare March 19, 2025 14:37
@thaJeztah thaJeztah force-pushed the simplify_auth branch 4 times, most recently from 074b5a7 to c784802 Compare March 25, 2025 12:25
@thaJeztah thaJeztah force-pushed the simplify_auth branch 3 times, most recently from a3f954e to 26845e4 Compare April 4, 2025 19:52
In most situations, the CLI is created through the `NewDockerCli` constructor,
however, it's possible to construct a CLI manually (`&DockerCli{}`). We
should probably prevent this (and un-export the `DockerCli` implementation),
but currently have some code-paths that depend on the type being exported.

When constructing the CLI with this approach, the CLI would not be fully
initialized and not have the context-store configuration set up.

 Using the default context store without a config set will result in Endpoints
 from contexts not being type-mapped correctly, and used as a generic
 `map[string]any`, instead of a [docker.EndpointMeta].

When looking up the API endpoint (using [EndpointFromContext]), no endpoint
will be found, and a default, empty endpoint will be used instead which in
its turn, causes [newAPIClientFromEndpoint] to be initialized with the default
config instead of settings for the current context (which may mean; connecting
with the wrong endpoint and/or TLS Config to be missing).

I'm not sure if this situation could happen in practice, but it caused some
of our unit-tests ([TestInitializeFromClient] among others) to fail when
running outside of the dev-container on a host that used Docker Desktop's
"desktop-linux" context. In that situation, the test would produce the wrong
"Ping" results (using defaults, instead of the results produced in the test).

This patch:

- updates the contextStoreConfig field to be a pointer, so that we are
  able to detect if a config was already set.
- updates the `Initialize` function to set the default context-store config
  if no config was found (technically the field is mostly immutable, and
  can only set through `WithDefaultContextStoreConfig`, so this may be
  slightly redundant).

We should update this code to be less error-prone to use; the combination
of an exported type (`DockerCli`), a constructor `NewDockerCli` and a
`Initialize` function (as well as some internal contructors to allow
lazy initialization) make constructing the "CLI" hard to use, and there's
various codepaths where it can be in a partially initialized state. The
same applies to the default context store, which also requires too much
"domain" knowledge to use properly.

I'm leaving improvements around that for a follow-up.

[EndpointFromContext]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/context/docker/load.go#L139-L149
[docker.EndpointMeta]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/context/docker/load.go#L19-L21
[newAPIClientFromEndpoint]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/command/cli.go#L295-L305
[TestInitializeFromClient]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/command/cli_test.go#L157-L205

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Lots to do here; too many wrappers everywhere, which may become easier
when content trust is removed (which adds another level of abstraction)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
These options were moved to opts/swarmopts in ad21055
and have no known external consumers.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants